Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve read_object speed when passing idx #35

Merged
merged 12 commits into from
Nov 9, 2023
Merged

Conversation

lvarriano
Copy link
Contributor

See #29 for details of issue and fix. "Fixes" this issue by sacrificing a small amount memory for additional speed. This can improve read_object speeds when using the idx parameter by more than x50 depending on how many events are selected from a file.

There is now a x2 speed penalty if the user passes in idx that corresponds to all events compared to just reading everything without any idx. In the future, read_object could check if the user did this. For now, however, it is up the user to check.

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Files Coverage Δ
src/lgdo/lh5_store.py 82.70% <85.00%> (+0.14%) ⬆️

📢 Thoughts on this report? Let us know!

@gipert gipert added performance Code performance lh5 HDF5 I/O labels Nov 9, 2023
@gipert
Copy link
Member

gipert commented Nov 9, 2023

We should still allow directly reading with slice objects. How would the user do that?

@gipert
Copy link
Member

gipert commented Nov 9, 2023

Ok nevermind, slices are used when setting start_row/n_rows.

@lvarriano
Copy link
Contributor Author

Thanks for the idea, Luigi! I changed it so that if idx can be converted to a slice instead (all indices are increasing by 1), then it will do that. This restores some of the speed if the user happens to pass in idx that corresponds to all events (now ~30% slowdown instead of x2). And this improved the speed of the original implementation.

@jasondet
Copy link
Contributor

jasondet commented Nov 9, 2023

looks very good to me. I'm going to merge.

@jasondet jasondet merged commit 7dfaf79 into legend-exp:main Nov 9, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lh5 HDF5 I/O performance Code performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants